Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add liveliness bindings #189

Merged
merged 1 commit into from
Nov 15, 2023
Merged

Conversation

sashacmc
Copy link
Member

@sashacmc sashacmc commented Nov 13, 2023

Add the liveliness methods bindings:

  • zc_liveliness_declaration_options_check
  • zc_liveliness_declaration_options_drop
  • zc_liveliness_declaration_options_null
  • zc_liveliness_declare_subscriber
  • zc_liveliness_declare_token
  • zc_liveliness_get
  • zc_liveliness_get_options_check
  • zc_liveliness_get_options_default
  • zc_liveliness_get_options_drop
  • zc_liveliness_get_options_null
  • zc_liveliness_subscriber_options_check
  • zc_liveliness_subscriber_options_drop
  • zc_liveliness_subscriber_options_null
  • zc_liveliness_token_check
  • zc_liveliness_token_null
  • zc_liveliness_undeclare_token

Along with the examples:

  • z_sub_liveliness
  • z_get_liveliness

Cargo.toml Outdated Show resolved Hide resolved
@Mallets
Copy link
Member

Mallets commented Nov 13, 2023

Hello @sashacmc , in order to merge the PR we will need you to sign the Eclipse Contribution Agreement.

@sashacmc sashacmc marked this pull request as ready for review November 14, 2023 17:28
*/
typedef struct zc_owned_liveliness_declaration_options_t {
uint8_t _inner;
} zc_owned_liveliness_declaration_options_t;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The zenoh-commons.h header is meant to be common across zenoh-c and zenoh-pico.
All functions starting with zc_ are inherently meant to be specific to zenoh-c.
All these function declarations should hence go into zenoh-concrete.h.

@p-avital @milyin I see that other zc_ functions slipped in zenoh-commons.h in the past. I think they should be moved into zenoh-concrete.h as well. Opinions?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the original goal was to be able to copy-paste this header into pico to ensure we always have the same signatures, but it didn't end up working out that way.

I think we can make the splitter logic a bit better if we want to stick to the original ideal. Otherwise, we could remove the splitter entirely, or just leave it in that state. Any odd these solutions is fine by me :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

zenoh-pico has evolved quite a bit since we initially made that split and I see how the split could hardly work out in practice. Therefore I'm more in favour of unifying all the includes in a single file rather than introducing artificial splits (although correct in principle).

For what concerns the scope of this specific PR, let's not change anything for the time being... A different PR may follow in the future.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, though note that we'll never be single filed because the zenoh-macros file is still hand-written

@Mallets Mallets merged commit 193fe45 into eclipse-zenoh:master Nov 15, 2023
3 checks passed
@Yadunund
Copy link

fyi I ran into some issues when relying on z_check and z_drop macros with the new liveliness structs. Had to rely on the zc_liveliness_XX functions to check my tokens and undeclare them. More details: ros2/rmw_zenoh#67

@p-avital
Copy link
Contributor

p-avital commented Nov 16, 2023

fyi I ran into some issues when relying on z_check and z_drop macros with the new liveliness structs. Had to rely on the zc_liveliness_XX functions to check my tokens and undeclare them. More details: ros2/rmw_zenoh#67

Is this the first time you've had issues with these macros? I seem to remember some incompatibility between C's generic macros and C++... The macros failing only on liveliness is strange considering the implementation of the macro is the same for them as for the rest...

@Yadunund
Copy link

fyi I ran into some issues when relying on z_check and z_drop macros with the new liveliness structs. Had to rely on the zc_liveliness_XX functions to check my tokens and undeclare them. More details: ros2/rmw_zenoh#67

Is this the first time you've had issues with these macros? I seem to remember some incompatibility between C's generic macros and C++... The macros failing only on liveliness is strange considering the implementation of the macro is the same for them as for the rest...

yup first time running into problems with the macros. I've been relying on them to check/drop owned sessions, publishers, subscribers without any problems.

@JEnoch
Copy link
Member

JEnoch commented Nov 20, 2023

@Yadunund the z_check macros for zc_owned_liveliness_token_t were indeed missing and are now available thanks to #195 .
I think the undefined symbol error calling z_drop() was probably an build environment issue.

I created ros2/rmw_zenoh#69 that hopefully will build successfully once workflows are approved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants